Skip to content

openssh: fix static build#100906

Merged
nh2 merged 2 commits intoNixOS:stagingfrom
KAction:openssh
Dec 8, 2020
Merged

openssh: fix static build#100906
nh2 merged 2 commits intoNixOS:stagingfrom
KAction:openssh

Conversation

@KAction
Copy link
Contributor

@KAction KAction commented Oct 18, 2020

This pull requests fixes static build of openssh and its dependency,
keyutils:

$ nix-build -A pkgsStatic.openssh

It seems that this is symptom of more general problem: if library "foo"
depends on library "bar", then to link dynamically program that uses
library "foo" it is enough to pass "-lfoo" to compiler, but it is
necessary to pass "-lfoo -lbar" (in that order) to compiler to link it
statically.

Maybe proper fix would be to add -l{bar} into NIX_LDFLAGS for every
{bar} in build closure when static build is requested
(stdenv.hostPlatform.isStatic)?

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@KAction KAction changed the title Openssh openssh: fix static build Oct 18, 2020
@ofborg ofborg bot requested review from aneeshusa and edolstra October 18, 2020 01:17
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Oct 18, 2020
#
# ~kaction
+ optionalString stdenv.hostPlatform.isStatic ''
export NIX_LDFLAGS="$NIX_LDFLAGS -lncurses"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing static builds is great, but this doesn't seem like the right way to do it.

  • NIX_LDFLAGS is a hack that should only be used as last resort. It sneaks flags past the package's build system, thus often making downstream packages not work as expected.
  • pkg-config should be able to figure that out, if used:
    % nix-shell -A openssh
    
    $ pkg-config --libs libedit
    -L/nix/store/sbfj9lhpfi5v9vwrdf9my8l1m7cwbi37-libedit-20191231-3.1/lib -ledit
    
    $ pkg-config --libs libedit --static
    -L/nix/store/sbfj9lhpfi5v9vwrdf9my8l1m7cwbi37-libedit-20191231-3.1/lib -ledit -lncurses

Notice how when --static is passed to pkg-config, it actually emits the required flags of the recursive dependencies.

The way that works is this:

$ echo $PKG_CONFIG_PATH | tr ':' '\n' | grep libedit | head -n1
/nix/store/wx8xws36cz66gabl9sapgaz9p70h7x1m-libedit-20191231-3.1-dev/lib/pkgconfig

$ cat /nix/store/wx8xws36cz66gabl9sapgaz9p70h7x1m-libedit-20191231-3.1-dev/lib/pkgconfig/libedit.pc
...
Libs: -L${libdir} -ledit
Libs.private: -lncurses 
...

Here we can see that the .pc file for libedit "remembers" its non-dynamic dependencies in Libs.private, so that when asked to be linked in statically by a downstream package, it can emit them.

This is pkg-config's direct way to address your observation:

It seems that this is symptom of more general problem: if library "foo"
depends on library "bar", then to link dynamically program that uses
library "foo" it is enough to pass "-lfoo" to compiler, but it is
necessary to pass "-lfoo -lbar" (in that order) to compiler to link it
statically.

So, the right way that this should work is that you tell openssh's build system to link itself statically, based on that it should invoke pkg-config with the --static flag, and then this manual addition of flags should not be necessary.

Clearly somewhere that process breaks down, and the best way forward is to investigate where.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really great explanation, I learned something new today about how pkg-config works. As openssh maintainer I would agree the current state of the PR doesn't look like the right way to solve things but no idea what the right approach is so thanks for stepping in and posting @nh2!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nh2 Thank you for review.

Unfortunately, openssh build system does not understand concept of static build, so I had to fall back on sed. Libedit was quite simple, since it used pkg-config, so I just replaced all calls to pkg-config with pkg-config --static.

Kerberos was harder. Despite library providing pkg-config files, build system uses either own shell code to figure out compiler flags. I added "kerberos.dev" into build inputs, so configure script control flow into the branch which is simpler to patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KAction Yes, that looks good, great!

Please add what you described here as a comment on top of your seds in the nix file though, so that we keep the rationale next to the code.

Your change here might actually help me with nh2/static-haskell-nix#68 which is nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KAction with the latest force-push

@KAction KAction force-pushed the KAction:openssh branch from 4879ea9 to 56a01c2 22 hours ago

you're moving back to the NIX_LDFLAGS approach -- just checking that it's intended.

I saw the updated commit message:

openssh: fix static, with-kerberos build

Error messages without this patch are quite misleading: they are
complaining about compilation where root of the problem is configure
stage.

Without these extra linking flags (mostly extracted from pkg-config
database), configure flag interprets undefined reference as missing
header/function error, resulting in problems at compile time.

so probably it's intended, and I think that's fine if there is no other way, but I'd definitely put that into a comment that explains that that is the reason why we're using NIX_LDFLAGS here for now, and also the concrete invocation that made you end up with those specific -l flags, so that we know how to update it.

Also, if you've already investigated that topic/failure a bit further, it would be great to post that info here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, pushed wrong branch. No, it is possible to get things working without NIX_LDFLAGS.

@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. labels Oct 19, 2020
@nh2
Copy link
Contributor

nh2 commented Oct 19, 2020

@KAction You'll have to base the PR against staging because it's a mass rebuild (you can do it by rebasing with git and then using Github's Edit button at the top).

@FRidh FRidh changed the base branch from master to staging October 20, 2020 19:00
@KAction
Copy link
Contributor Author

KAction commented Oct 20, 2020

@nh2 Rebased on staging, @FRidh changes target branch for me.

@expipiplus1
Copy link
Contributor

It would be great to get this merged!

@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. labels Nov 25, 2020
@nh2
Copy link
Contributor

nh2 commented Nov 25, 2020

It would be great to get this merged!

I just replied #100906 (comment)

@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. labels Nov 28, 2020
@KAction
Copy link
Contributor Author

KAction commented Nov 28, 2020

@nh2 Sorry for confusion. I believe it is now ready for merging.

@FRidh FRidh requested a review from nh2 December 8, 2020 04:19
@nh2 nh2 merged commit 87413f3 into NixOS:staging Dec 8, 2020
@lopsided98
Copy link
Contributor

lopsided98 commented Dec 25, 2020

This breaks cross-compilation because krb5-config gets taken from the build platform package. *-config scripts are generally problematic for cross-compilation, but in this case it should work if we add host platform kerberos.dev to nativeBuildInputs, or just manually add krb5-config to PATH. This works because krb5-config is just a shell script.

lopsided98 added a commit to lopsided98/nixpkgs that referenced this pull request Dec 25, 2020
krb5-config from the host platform needs to be added to PATH so it can be run
during build. This works because krb5-config is a platform independent
shell-script. Before NixOS#100906, krb5-config was not used, so we didn't run into
this problem.
@lopsided98 lopsided98 mentioned this pull request Dec 26, 2020
10 tasks
@KAction KAction deleted the openssh branch October 29, 2021 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants